Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

BoringSSL support #814

Merged
merged 3 commits into from
Mar 13, 2024
Merged

BoringSSL support #814

merged 3 commits into from
Mar 13, 2024

Conversation

johnhe4
Copy link

@johnhe4 johnhe4 commented Jan 13, 2024 •
edited
Loading

BoringSSL support

This is a PR to compile/link RabbitMQ against either BoringSSL or OpenSSL.

Why BoringSSL?

Consider an Android static library that depends on both librabbitmq and libcurl.

graph TD;
   myLib.a--> librabbitmq.a;
   myLib.a-->libcurl.a;
   librabbitmq.a-->libssl.a+libcrypto.a;
   libcurl.a-->libssl.a+libcrypto.a;
Loading

OpenSSL isn't the best option because it doesn't naturally support the certificates on an Android device. Forcing it to work involves either including certificates with mylib.a or modifying the user's file system.

libcurl expects to link against BoringSSL for Android, but RabbitMQ only compiles/links against OpenSSL. BoringSSL is a fork from OpenSSL somylib.a cannot compile/link against both without getting collisions.

I have verified that BoringSSL works out-of-the-box on Android without any hacking for libcurl, and that this PR allows librabbitmq to work with BoringSSL in my limited tests. Backward compatibility with OpenSSL has been maintained.

@alanxz
Copy link
Owner

alanxz commented Jan 20, 2024

Not sure about this PR specifically, but answering some of the questions

How to select between BoringSSL and OpenSSL? Always for Android? CMAKE variable? Other mechanism?

If you know where BoringSSL is installed, you can specify OPENSSL_ROOT_DIR CMake variable pointing to that path, and the FindOpenSSL.cmake thing will find it.

How to specify a BoringSSL version? Is this even important? The cmake variable OPENSSL_VERSION was not populated when I tested this patch.

BoringSSL doesn't seem have specified releases. As a result OPENSSL_VERSION will likely not be specified.

@johnhe4
Copy link
Author

johnhe4 commented Jan 22, 2024

I'll add some clarity to the issues my PR doesn't address:

  • In my testing, find_package fails to find boringssl if any version is specified, regardless of OPENSSL_ROOT_DIR being specified or not. My PR comments out the min version requirement in order to get find_package to work for boringssl. I imagine RMQ_OPENSSL_MIN_VERSION was added for a reason so simply ignoring it seems short-sighted.
  • the BIO_* interface was updated for openssl 1.1.something, but boringssl is still using the old interface. The code in amqp_openssl_bio.c needs to be conditional depending on which ssl library is used, unless openssl is backwards compatible (I haven't tested that)

@johnhe4
Copy link
Author

johnhe4 commented Mar 13, 2024

  • Addressed the version check issue by finding openssl without a version, then manually checking the version if it exists. This results in a version check for OpenSSL but not for BoringSSL which is the desired outcome.
  • Using the preprocessor flag OPENSSL_IS_BORINGSSL to determine whether or not we need BoringSSL BIO code or OpenSSL BIO code

These changes address the concerns listed above.

@johnhe4 johnhe4 changed the title BoringSSL patch, not ready for merge BoringSSL support Mar 13, 2024
@johnhe4
Copy link
Author

johnhe4 commented Mar 13, 2024

@alanxz Any idea why the 'tsan' test is failing? I cannot make a connection between these changes and that failure.

@alanxz
Copy link
Owner

alanxz commented Mar 13, 2024

I think something to do with the github actions runner has changed (unrelated to this pull), doing a re-run on the individual job seems to make it work.

@alanxz
Copy link
Owner

alanxz commented Mar 13, 2024

As a note for this PR: BoringSSL doesn't promise API stability (see: https://github.com/google/boringssl/blob/master/README.md), so while I will accept this, I won't promise stability going forward.

@alanxz alanxz merged commit 5ec806e into alanxz:master Mar 13, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@johnhe4 @alanxz